Skip to content

refactor(clientonly): modernize code structure and add comprehensive tests#4022

Merged
khassel merged 3 commits intoMagicMirrorOrg:developfrom
KristjanESPERANTO:clientonly
Jan 28, 2026
Merged

refactor(clientonly): modernize code structure and add comprehensive tests#4022
khassel merged 3 commits intoMagicMirrorOrg:developfrom
KristjanESPERANTO:clientonly

Conversation

@KristjanESPERANTO
Copy link
Copy Markdown
Collaborator

This PR improves clientonly start option with better code structure, validation, and comprehensive test coverage.

Changes

Refactoring:

  • Improved parameter handling with explicit function parameter passing instead of closure
  • Added port validation (1-65535) with proper NaN handling
  • Removed unnecessary IIFE wrapper (Node.js modules are already scoped)
  • Extracted getCommandLineParameter as a reusable top-level function
  • Enhanced error reporting with better error messages
  • Added connection logging for debugging

Testing:

  • Added comprehensive e2e tests for parameter validation
  • Test coverage for missing/incomplete parameters
  • Test coverage for local address rejection (localhost, 127.0.0.1, ::1, ::ffff:127.0.0.1)
  • Test coverage for port validation (invalid ranges, non-numeric values)
  • Test coverage for TLS flag parsing
  • Integration test with running server

Testing

All tests pass:

npm test -- tests/e2e/clientonly_spec.js
# ✓ 18 tests passed

- Test missing/incomplete parameters (address, port)
- Test rejection of local addresses (localhost, 127.0.0.1, ::1, etc.)
- Test port validation (0, negative, >65535, non-numeric)
- Test --use-tls flag parsing
- Test server /config/ endpoint availability
- Remove IIFE (Node.js modules are already scoped)
- Extract getCommandLineParameter as top-level function
- Add connection logging for better debugging
@khassel
Copy link
Copy Markdown
Collaborator

khassel commented Jan 28, 2026

Not directly related to this pull request, but I've been wondering whether we need these subdirectories for clientonly and serveronly. We could put everything under js and simply change the call in package.json for serveronly. And we could do the same for clientonly.

But maybe I've overlooked something that argues against it...

@KristjanESPERANTO
Copy link
Copy Markdown
Collaborator Author

I'm a bit divided on this.

For client-only users, this would be a breaking change. Until now, they have been using
node clientonly --address 192.168.1.5 --port 8080
to start. After a migration like suggested, it would be
node js/client.js --address 192.168.1.5 --port 8080 or
node --run client -- --address 192.168.1.5 --port 8080.

For server users it would stay the same command.

@khassel
Copy link
Copy Markdown
Collaborator

khassel commented Jan 28, 2026

yes, I know that it would be a breaking change for client users, so we better don't change it.

@KristjanESPERANTO
Copy link
Copy Markdown
Collaborator Author

Yes, if we had strong arguments for a breaking change, we could certainly make the change, but I don't see them here yet.

@khassel khassel merged commit 2b55b8e into MagicMirrorOrg:develop Jan 28, 2026
11 of 12 checks passed
@KristjanESPERANTO KristjanESPERANTO deleted the clientonly branch January 28, 2026 19:59
@khassel khassel mentioned this pull request Apr 1, 2026
khassel added a commit that referenced this pull request Apr 1, 2026
## Release Notes
Thanks to: @angeldeejay, @in-voker, @JHWelch, @khassel,
@KristjanESPERANTO, @rejas, @sdetweil
> ⚠️ This release needs nodejs version >=22.21.1 <23 || >=24 (no change
to previous release)

[Compare to previous Release
v2.34.0](v2.34.0...v2.25.0)

> ⚠️ We introduced some internal changes with this release, please read
[this forum
post](https://forum.magicmirror.builders/topic/20138/upcoming-release-april-1-2026-breaking-changes-some-operational-changes)
before upgrading!

### [core]
- Prepare Release 2.35.0 (#4071)
- docs: add security policy and vulnerability reporting guidelines
(#4069)
- refactor: simplify internal `require()` calls (#4056)
- allow environment variables in cors urls (#4033)
- fix cors proxy getting binary data (e.g. png, webp) (#4030)
- fix: correct secret redaction and optimize loadConfig (#4031)
- change loading config.js, allow variables in config.js and try to
protect sensitive data (#4029)
- remove kioskmode (#4027)
- Add dark theme logo (#4026)
- move custom.css from css to config (#4020)
- move default modules from /modules/default to /defaultmodules (#4019)
- update node versions in workflows (#4018)
- [core] refactor: extract and centralize HTTP fetcher (#4016)
- fix systeminformation not displaying electron version (#4012)
- Update node-ical and support it's rrule-temporal changes (#4010)
- Change default start scripts from X11 to Wayland (#4011)
- refactor: unify favicon for index.html and Electron (#4006)
- [core] run systeminformation in subprocess so the info is always
displayed (#4002)
- set next release dev number (#4000)

### [dependencies]
- update dependencies (#4068)
- update dependencies incl. electron to v41 (#4058)
- chore: upgrade ESLint to v10 and fix newly surfaced issues (#4057)
- chore: update ESLint and plugins, simplify config, apply new rules
(#4052)
- chore: update dependencies + add exports, files, and sideEffects
fields to package.json (#4040)
- [core] refactor: enable ESLint rule require-await and handle detected
issues (#4038)
- Update node-ical and other deps (#4025)
- chore: update dependencies (#4021)
- chore(eslint): migrate from eslint-plugin-vitest to
@vitest/eslint-plugin and run rules only on test files (#4014)
- Update deps as requested by dependabot (#4008)
- update Collaboration.md and dependencies (#4001)

### [logging]
- refactor: further logger clean-up (#4050)
- Fix Node.js v25 logging prefix and modernize logger (#4049)

### [modules/calendar]
- fix(calendar): make showEnd behavior more consistent across time
formats (#4059)
- test(calendar): fix hardcoded date in event shape test (#4055)
- [calendar] refactor: delegate event expansion to node-ical's
expandRecurringEvent (#4047)
- calendar.js: remove useless hasCalendarURL function (#4028)
- fix(calendar): update to node-ical 0.23.1 and fix full-day recurrence
lookup (#4013)
- fix(calendar): correct day-of-week for full-day recurring events
across all timezones (#4004)

### [modules/newsfeed]
- fix(newsfeed): fix full article view and add framing check (#4039)
- [newsfeed] refactor: migrate to centralized HTTPFetcher (#4023)

### [modules/weather]
- fix(weather): fix openmeteo forecast stuck in the past (#4064)
- fix(weather): fix weathergov forecast day labels off by one (#4065)
- weather: fixes for templates (#4054)
- weather: add possibility to override njk's and css (#4051)
- Use getDateString in openmeteo (#4046)
- [weather] refactor: migrate to server-side providers with centralized
HTTPFetcher (#4032)
- [weather] feat: add Weather API Provider  (#4036)

### [testing]
- chore: remove obsolete Jest config and unit test global setup (#4044)
- replace template_spec test with config_variables test (#4034)
- refactor(clientonly): modernize code structure and add comprehensive
tests (#4022)
- Switch to undici Agent for HTTPS requests (#4015)
- chore: migrate CI workflows to ubuntu-slim for faster startup times
(#4007)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
Co-authored-by: Bugsounet - Cédric <github@bugsounet.fr>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: sam detweiler <sdetweil@gmail.com>
Co-authored-by: Veeck <github@veeck.de>
Co-authored-by: veeck <gitkraken@veeck.de>
Co-authored-by: Magnus <34011212+MagMar94@users.noreply.github.com>
Co-authored-by: Ikko Eltociear Ashimine <eltociear@gmail.com>
Co-authored-by: DevIncomin <56730075+Developer-Incoming@users.noreply.github.com>
Co-authored-by: Nathan <n8nyoung@gmail.com>
Co-authored-by: mixasgr <mixasgr@users.noreply.github.com>
Co-authored-by: Savvas Adamtziloglou <savvas-gr@greeklug.gr>
Co-authored-by: Konstantinos <geraki@gmail.com>
Co-authored-by: OWL4C <124401812+OWL4C@users.noreply.github.com>
Co-authored-by: BugHaver <43462320+bughaver@users.noreply.github.com>
Co-authored-by: BugHaver <43462320+lsaadeh@users.noreply.github.com>
Co-authored-by: Koen Konst <koenspero@gmail.com>
Co-authored-by: Koen Konst <c.h.konst@avisi.nl>
Co-authored-by: dathbe <github@beffa.us>
Co-authored-by: Marcel <m-idler@users.noreply.github.com>
Co-authored-by: Kevin G. <crazylegstoo@gmail.com>
Co-authored-by: Jboucly <33218155+jboucly@users.noreply.github.com>
Co-authored-by: Jboucly <contact@jboucly.fr>
Co-authored-by: Jarno <54169345+jarnoml@users.noreply.github.com>
Co-authored-by: Jordan Welch <JordanHWelch@gmail.com>
Co-authored-by: Blackspirits <blackspirits@gmail.com>
Co-authored-by: Samed Ozdemir <samed@xsor.io>
Co-authored-by: in-voker <58696565+in-voker@users.noreply.github.com>
Co-authored-by: Andrés Vanegas Jiménez <142350+angeldeejay@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants